Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gbn+mailbox: cleanup & prefixed logger #90

Merged
merged 4 commits into from
Nov 28, 2023
Merged

gbn+mailbox: cleanup & prefixed logger #90

merged 4 commits into from
Nov 28, 2023

Conversation

ellemouton
Copy link
Member

@ellemouton ellemouton commented Nov 20, 2023

This PR introduces a prefixed logger which is then passed to the client and server so that
their logs are prefixed with (client) and (server) so that we dont have to manually use
the isServer variable in logs.

Then, some variables are moved out of the GBN struct and into a new config struct.

depends on #92

Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this!! This will be really, really helpful 🙏:fire:!!

Left a few comments below, but also leaving one extra feedback here:

Should we change the mailbox/server_conn.go & mailbox/client_conn.go to also use a prefixed logger? Those structs currently have quite a few log calls that adds a separate "Server" & "Client" prefix.

Also, should I change my packeting bug PRs to be based on this already?

gbn/gbn_conn.go Outdated Show resolved Hide resolved
gbn/gbn_server.go Outdated Show resolved Hide resolved
gbn/queue.go Show resolved Hide resolved
gbn/config.go Show resolved Hide resolved
@ellemouton ellemouton force-pushed the gbnCleanup branch 3 times, most recently from 5fa6e72 to ef7b4fc Compare November 22, 2023 12:11
Copy link
Member Author

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ViktorTigerstrom ! Added the prefixed logger for the mailbox package as you suggested 🙏

gbn/gbn_conn.go Outdated Show resolved Hide resolved
gbn/config.go Show resolved Hide resolved
@ellemouton ellemouton changed the title gbn: cleanup gbn+mailbox: cleanup & prefixed logger Nov 22, 2023
@ellemouton
Copy link
Member Author

Also, should I change my packeting bug PRs to be based on this already?

Yeah I think that would be a good idea just so that you get the advantages of the prefixed logger & so that you can take advantage of the slightly cleaner GBN structure.

You can change the base of you PR to gbnCleanup if you want (we must just not forget to change the base of that one before deleting the branch ref after merge)

Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK LGTM, thanks for this 🙏🔥!!

mailbox/client_conn.go Outdated Show resolved Hide resolved
gbn/gbn_server.go Outdated Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Just a couple of nits, otherwise LGTM 🎉

gbn/gbn_conn.go Outdated Show resolved Hide resolved
gbn/gbn_client.go Show resolved Hide resolved
mailbox/client_conn.go Outdated Show resolved Hide resolved
Instead of manually needing to insert "isServer=" everywhere.
Copy link
Member Author

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Y'all 🤓

gbn/gbn_client.go Show resolved Hide resolved
@ellemouton ellemouton merged commit fa60171 into master Nov 28, 2023
5 checks passed
@ellemouton ellemouton deleted the gbnCleanup branch November 28, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants